Fix: reformat long seqs #806
Conversation
There was a problem hiding this comment.
@joko3ono thanks for looking into this. Could you please add an explanation of what was the issue and how your change fixes the issue to the commit message, and also add some jest unit tests to demonstrate how it works + make sure we don't break it in future?
It's a bit hard to review the changes without context and learning what the bug is from scratch would be very time consuming as I'd have to come up with multiple test scenarios and execute them 😬
|
things look almost ok however, there needs to be correct rounding up of the final sequence length. See below that:
However...If we want to show a greater level of precision then we can show to at least 1 decimal place for any value that has units kbp, Mbp and Gbp. ie
divide these numbers by 1000000 and round to the nearest decimal place
|
|
@joko3ono also please make sure you add tests for the rounding so that it's not broken in future |
joko3ono
force-pushed
the
415-fix-incorrect-length-of-sequence
branch
from
e8d0b07
to
103b934
Compare
| // threshold 0.95 | ||
| // 2.95 -> 3 | ||
| // 2.94 -> 2.9 | ||
| const formatted = fractionalPart >= threshold ? integerPart + 1 : Math.floor(numericPart * 10) / 10; |
There was a problem hiding this comment.
Do we need this custom rounding logic threshold? Am I right in thinking that we could instead round up to 1 decimal place, then remove ".0" at the end if it's a whole number? I think it would achieve the same result with a much easier to understand and simpler code, or am I missing something?
There was a problem hiding this comment.
@tadast the threshold based on suggested logic from Daniel
divide these numbers by 1000 and round to the nearest decimal place
19,299 bp = 19.3 kbp
19,990 bp = 20.0 kbp
divide these numbers by 1000000 and round to the nearest decimal place199,929,999 bp = 199.9 Mbp
199,990,000 bp = 200.0 Mbp
There was a problem hiding this comment.
I think what I said in the previous comment still works. We just want to round up to the nearest decimal place and remove /\.0$/
| @@ -0,0 +1,28 @@ | |||
| // Import the necessary functions | |||
| import { tick_formatter, formatNumberUnits } from '../visualisation_helpers'; | |||
There was a problem hiding this comment.
Thanks for adding the tests!
resolve wurmlab/sequenceserver-cloud#415